Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use more strict rules for identifiers #25

Merged
merged 2 commits into from
Jul 22, 2015
Merged

Conversation

dirk-thomas
Copy link
Member

The patch enables the stricter rules (instead of using the relaxed one to be able to read all ROS 1 messages). It also makes the regular expressions even more strict to match the spec from the article.

Please review.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Jun 17, 2015
@dirk-thomas dirk-thomas self-assigned this Jun 17, 2015
@wjwwood
Copy link
Member

wjwwood commented Jun 17, 2015

+1

2 similar comments
@tfoote
Copy link
Contributor

tfoote commented Jun 18, 2015

+1

@esteve
Copy link
Member

esteve commented Jun 18, 2015

+1

@wjwwood
Copy link
Member

wjwwood commented Jun 18, 2015

So I've finally run into the messages which lead us to have the "less" strict regex. For example, sensor_msgs/CameraInfo:

http://docs.ros.org/api/sensor_msgs/html/msg/CameraInfo.html

It has fields called D, K, R, and P. These are "symbols" that represent elements of the image rectification algorithm. You could call them scientific or mathematical terms. So, the question is what to do about these? I guess we could update CameraInfo's field names, and I'd say that the plain english versions of them might even be more readable and informative, but I'm not sure about enforcing this across all messages. It seems like that might be too strict in some cases.

If we decide to keep this enforcement then I'll need help figuring out how to change CameraInfo correctly. At the very least I think changing CameraInfo in this way will require iteration with the community and I'm sure it will make sense to them why we are doing this. I have to admit that "To match the style" might not be justification enough, even for me.

@dirk-thomas
Copy link
Member Author

For the fields in the CameraInfo message I would rename them to lower case. I would assume that everybody familiar with the symbols will directly understand there meaning.

@wjwwood
Copy link
Member

wjwwood commented Jun 18, 2015

That might be the best solution in this case. However, +0 to changing CameraInfo like this. I think we need to discuss it with the community.

@tfoote
Copy link
Contributor

tfoote commented Jun 23, 2015

From discussion in meeting we'll go ahead with this and see how painful the conversions are with this policy in place.

@wjwwood
Copy link
Member

wjwwood commented Jun 23, 2015

For me to do, update messages that violate this stricter policy (CameraInfo).

@gerkey
Copy link
Member

gerkey commented Jul 7, 2015

Feedback is that the benefit from consistent style is outweighed by the cost of having to update existing message definitions and code using them. So we'll provide a linter that you can opt out of using.

Open question: how should we resolve existing violations of this style in widely used messages (of which CameraInfo is one)? @esteve to reply to the email thread about this.

@gerkey
Copy link
Member

gerkey commented Jul 14, 2015

For now, we'll keep the strict version of the generators, and we'll update ROS2 versions of CameraInfo and DisparityImage to follow the new style, just lower-casing them.

In the future, we can consider adding a feature to the generators to make them less strict, if there's enough demand for it.

Also, in the future we can consider proposals for changing those (and other) field names to something more descriptive.

@wjwwood to update the messages.

Then, this PR is ready to merge.

Then, @dirk-thomas to update the bridge to handle this new mapping.

@wjwwood
Copy link
Member

wjwwood commented Jul 14, 2015

@esteve
Copy link
Member

esteve commented Jul 15, 2015

Given that several members of the ROS community have expressed their concerns about this proposal, I think we should close this PR and work on the linter instead.

@wjwwood
Copy link
Member

wjwwood commented Jul 21, 2015

I've opened the pr to change the offending messages: ros2/common_interfaces#6

@wjwwood
Copy link
Member

wjwwood commented Jul 21, 2015

I will change the branch name in my pr and run CI to double check before merging.

@wjwwood
Copy link
Member

wjwwood commented Jul 21, 2015

I create a duplicate branch for ros2/common_interfaces#6 to match this one's branch name and queued a CI for it: http://ci.ros2.org/job/ros2_batch_ci_linux/67/

@wjwwood
Copy link
Member

wjwwood commented Jul 21, 2015

I rebased this branch and started a new CI job: http://ci.ros2.org/job/ros2_batch_ci_linux/68/

@wjwwood
Copy link
Member

wjwwood commented Jul 22, 2015

The job passed, but with two failing tests. The tests are just style related though:

http://ci.ros2.org/job/ros2_batch_ci_linux/68/testReport/

After that is fixed up, I think this is ready to go.

@dirk-thomas
Copy link
Member Author

I updated the style in dc91c2f
Should be ready to go.

dirk-thomas added a commit that referenced this pull request Jul 22, 2015
use more strict rules for identifiers
@dirk-thomas dirk-thomas merged commit 7f0d9e1 into master Jul 22, 2015
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Jul 22, 2015
@dirk-thomas dirk-thomas deleted the more_strict_rules branch July 22, 2015 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants